Skip to content

feat(core): Wire TurboModulePerfLogger on iOS and Android#6307

Open
alwx wants to merge 28 commits into
mainfrom
alwx/feature/turbo-module-perf-logger
Open

feat(core): Wire TurboModulePerfLogger on iOS and Android#6307
alwx wants to merge 28 commits into
mainfrom
alwx/feature/turbo-module-perf-logger

Conversation

@alwx

@alwx alwx commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Install a Sentry-owned facebook::react::NativeModulePerfLogger on both platforms so the SDK observes every TurboModule lifecycle event:

  • moduleDataCreate{Start,End}, moduleCreate{Start,CacheHit,Construct*,SetUp*,End,Fail}
  • moduleJSRequireBeginning*, moduleJSRequireEnding*
  • syncMethodCall{Start,ArgConversion*,Execution*,ReturnConversion*,End,Fail}
  • asyncMethodCall{Start,ArgConversion*,Dispatch,End,Fail}
  • asyncMethodCallBatchPreprocess{Start,End}
  • asyncMethodCallExecution{Start,ArgConversion*,End,Fail}

This is the foundation that the next three issues in the Turbo Modules instrumentation project build on: JS↔Native crash attribution, per-Turbo-Module spans, and aggregated per-module stats. Each will ship its own ISentryTurboModulePerfSink implementation and plug into the hook this PR exposes.

New enableTurboModuleTracking option on Sentry.init, default false for this first release so the foundation lands without behavioural change. The native logger is always installed (we never want to miss early lifecycle events); the flag only decides whether forwarded callbacks reach the sink. The option is plumbed through initNativeSdk on both platforms.

Sentry.init({
  dsn: "___PUBLIC_DSN___",
  enableTurboModuleTracking: true, // off by default — will activate the follow-up sinks once they ship
});

💡 Motivation and Context

Closes #6162.

💚 How did you test it?

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
    • Docs deferred until a sink ships and the option becomes user-visible.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

This is the foundation for the Turbo Modules project. Follow-up issues plug in ISentryTurboModulePerfSink implementations:

  1. JS↔Native crash attribution — annotate native crashes with turbo_module.name / turbo_module.method of the call that was in flight.
  2. Per-Turbo-Module spans — open a span around each native call with duration, status, module.method data.
  3. Aggregated per-module stats — counters / duration histograms per module/method, attached to transactions.

Install a Sentry-owned `facebook::react::NativeModulePerfLogger` on
both platforms so the SDK observes every TurboModule lifecycle event \u2014
`moduleDataCreate*`, `moduleCreate*`, sync/async method call
`start`/`end`/`fail`, async dispatch and execution `start`/`end`/`fail`
\u2014 for follow-up features (crash attribution, per-module spans,
aggregated stats) to plug into.

The implementation is split into:

- **Shared C++** (`packages/core/cpp/`): a single
  `SentryTurboModulePerfController` singleton owns the installed logger
  and an atomic `enabled` flag. When disabled, every callback hits one
  atomic load and returns. When enabled, callbacks are forwarded to a
  swappable `ISentryTurboModulePerfSink` \u2014 follow-up issues ship the
  sinks; this PR just exposes the hook.

- **iOS**: the perf logger is installed from a dedicated installer
  class's `+load` so it fires before `RCTBridge` / `RCTHost` create
  their first TurboModule. (`RNSentry`'s own `+load` is reserved by
  `RCT_EXPORT_MODULE()`.) The cpp/ directory is added to the podspec
  sources; files are guarded with `RCT_NEW_ARCH_ENABLED` so Old Arch
  builds compile to empty TUs.

- **Android**: a new `libsentry-tm-perf-logger.so` shared library is
  built via CMake under New Architecture only and exposes `JNI_OnLoad`
  + a tiny `nativeSetEnabled` JNI hook. It links against React
  Native's `reactnative` prefab; the missing
  `<reactperflogger/NativeModulePerfLogger.h>` header is plugged by
  pointing the include path at the source tree (mirroring how
  react-native-reanimated resolves react-native via the standard
  `REACT_NATIVE_NODE_MODULES_DIR` / `require.resolve` fallback).
  `RNSentryPackage`'s static initializer `System.loadLibrary`s the
  perf-logger lib \u2014 host apps do NOT need to touch their own
  `OnLoad.cpp`. A guarded `try { \u2026 } catch (UnsatisfiedLinkError)`
  keeps Old Architecture (and any host that strips the lib) working
  as before.

Runtime gate: new `enableTurboModuleTracking` option on `Sentry.init`,
default `false` for this first release so the foundation lands without
behavioral change. The native logger is always installed (we never want
to miss early lifecycle events), the flag only decides whether
forwarded callbacks reach the Sentry sink. The option is plumbed
through `initNativeSdk` on both platforms.

Foundation only \u2014 no sink is installed in this PR. Follow-up issues
ship the actual instrumentation.

Closes #6162
@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Semver Impact of This PR

None (no version bump detected)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


  • feat(core): Wire TurboModulePerfLogger on iOS and Android by alwx in #6307
  • chore(deps): update Cocoa SDK to v9.19.0 by github-actions in #6343
  • chore(deps): update Android SDK to v8.45.0 by github-actions in #6344
  • fix(core): Align SENTRY_ENVIRONMENT/RELEASE/DIST in JS bundled options with native SDKs by antonis in #6330
  • chore(deps): update JavaScript SDK to v10.60.0 by github-actions in #6332
  • chore(core): Bump react-native and metro devDependencies to 0.86.0 by antonis in #6316
  • chore: Bump macOS sample to react-native-macos 0.81.7 by antonis in #6315
  • chore(e2e): Bump react-native devDependency to 0.86.0 by antonis in #6313
  • fix(core): Forward user geo as an object so the native scope keeps it by antonis in #6309
  • fix(ios): Remove manual geo handling, use sentry-cocoa native support by antonis in #6289
  • fix(deps): bump js-yaml from ^4.1.1 to ^4.2.0 by antonis in #6298
  • fix: update React Native repo URL to new GitHub org by antonis in #6290
  • chore(deps): update JavaScript SDK to v10.59.0 by github-actions in #6321
  • chore(deps): bump concurrent-ruby from 1.3.6 to 1.3.7 in /samples/react-native-macos by dependabot in #6327
  • chore(deps): bump undici from 6.24.1 to 6.27.0 by dependabot in #6328
  • chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 by dependabot in #6324
  • fix(ios): [RN 0.87] remove unused React/RCTTextView.h import by cortinico in #6322
  • chore(deps): bump actions/setup-java from 5.2.0 to 5.3.0 by dependabot in #6326
  • chore(deps): bump ruby/setup-ruby from 1.313.0 to 1.314.0 by dependabot in #6325
  • chore(deps): update Android SDK to v8.44.1 by github-actions in #6323

🤖 This preview updates automatically when you update the PR.

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor
Warnings
⚠️

⚠️ Android SDK Version Mismatch

Component Version
sentry-android in build.gradle 8.45.0
sentry-android bundled by gradle plugin 6.12.0 8.44.0

If a user enables AGP autoInstallation (default: true), this mismatch causes:

IllegalStateException: Sentry SDK has detected a mix of versions

Our docs instruct React Native users to set autoInstallation.enabled = false, so users following the guide are unaffected. Consider either updating packages/core/android/build.gradle to 8.44.0 or waiting for a gradle plugin release that bundles 8.45.0.

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against adef511

Comment thread packages/core/cpp/SentryTurboModulePerfLogger.h Outdated
@alwx

alwx commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 3b618c5. Configure here.

Address Warden's medium-severity finding on PR #6307: the new
`SentryTurboModulePerfController` and `RNSentryTurboModulePerfTracker`
shipped without unit coverage. Add focused tests that exercise the
state machines independently of React Native's runtime.

- **iOS** (`RNSentryCocoaTester/.../RNSentryTurboModulePerfControllerTests.mm`):
  default `isEnabled() == false`, `setEnabled` toggle, the C-linkage
  `Sentry_SetTurboModuleTrackingEnabled` entry point matches the typed
  setter, `setSink`/`sink` round-trips including `nullptr` detach,
  and `Sentry_InstallTurboModulePerfLogger` idempotency under repeated
  calls. End-to-end forwarding through `facebook::react::TurboModulePerfLogger`
  is intentionally not covered here \u2014 it requires `+load` ordering and
  process-wide singletons that the follow-up sink PRs will integration-test.

- **Android** (`RNSentryAndroidTester/.../RNSentryTurboModulePerfTrackerTest.kt`):
  the JVM-side latch around the JNI symbol. In the test JVM the
  underlying `.so` is not loaded, so the first `setEnabled` call must
  catch `UnsatisfiedLinkError` and flip `nativeUnavailable`; subsequent
  calls must short-circuit. Uses Robolectric so the `android.util.Log.i`
  call inside the catch branch resolves instead of throwing the
  not-mocked stub. A small `@TestOnly` window on the tracker exposes
  the latch state to assertions.

Also fix the changelog entry to reference the PR (#6307) rather than
the issue (#6162) so danger stops nagging.
@alwx alwx marked this pull request as ready for review June 18, 2026 08:49
Comment thread packages/core/cpp/SentryTurboModulePerfLogger.cpp Outdated
@alwx alwx enabled auto-merge (squash) June 18, 2026 09:40
@alwx alwx disabled auto-merge June 18, 2026 09:40
Comment thread packages/core/ios/RNSentry.mm Outdated
Comment thread packages/core/android/build.gradle Outdated

@antonis antonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for you work on this @alwx 🙇
Did a 1st pass and didn't notice anything off other what has been caught by the agents and the lint check. Let's also add the ready-to-merge since there are many changes on the native side that need to be validated.

…g flips on

Address two related medium findings on #6307:

- Warden: `enableLogging` runs from `+load` / `JNI_OnLoad` regardless
  of the runtime flag, unconditionally evicting any pre-existing
  `NativeModulePerfLogger` (Metro, other SDKs, host-app instrumentation).
- Cursor: when `enableTurboModuleTracking: true`, callbacks between
  load time and `initNativeSdk` are dropped by the `enabled_=false`
  fast-path anyway, so the eager install was not actually delivering
  on its 'never miss early events' promise \u2014 just on its side effects.

The fix is a single one-way ratchet: `setEnabled(true)` lazily calls
`install()` on the first transition, and the typed setter doubles as
the public lifecycle hook. The `+load` installer class on iOS and the
`JNI_OnLoad` install on Android are gone; the C `Sentry_InstallTurboModulePerfLogger`
entry stays for hosts that want to claim the perf-logger slot eagerly
via their own native code, but it is no longer wired into our load
hooks. Header / JSDoc updated to describe the new contract.

Also fix two adjacent issues flagged on the same PR:

- Sentry HIGH (build.gradle): two sibling `buildFeatures { ... }`
  blocks under the same Android scope replace rather than merge, so
  `prefab = true` was clobbering `buildConfig = true` on AGP 8+.
  Merge into a single conditional block.
- Lint: ran `yarn java:format fix`, `yarn fix:clang`, and switched
  `RNSentryTurboModulePerfTracker.nativeUnavailable` from `volatile`
  to `AtomicBoolean` to satisfy the project-wide PMD `AvoidUsingVolatile`
  rule. Removed a Kotlin `no-consecutive-comments` violation from the
  Robolectric note above the tracker test.

Test updates:
- iOS: add `testSetEnabledFalseDoesNotInstall` and
  `testSetEnabledTrueIsLazyInstallAndSticky` to lock down the lazy
  install ratchet. Existing `testInstallIsIdempotent` still covers
  explicit-install callers.
- Android: tracker tests unchanged in behaviour; only the test-only
  `isNativeUnavailableForTests` / `resetNativeUnavailableForTests`
  helpers were updated to go through the new `AtomicBoolean`.
@alwx alwx requested a review from antonis June 22, 2026 08:55
@alwx alwx added the ready-to-merge Triggers the full CI test suite label Jun 22, 2026
Comment thread packages/core/cpp/SentryTurboModulePerfLogger.cpp Outdated
Address Cursor's low-severity finding on #6307: `setEnabled(true)`
was storing `enabled_` *after* calling `install()`, so any callback
React Native fired synchronously from inside `enableLogging()` would
hit the `isEnabled() == false` fast-path and be dropped \u2014 a tiny
window of lost events for the very first opted-in invocation.

Swap the order: publish `enabled_ = true` (release ordering) before
the install, so by the time `enableLogging()` could re-enter us via a
synchronous callback, the flag is already visible to other threads.
On disable the order does not matter since we never uninstall.
@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 436.69 ms 470.69 ms 34.01 ms
Size 49.74 MiB 55.06 MiB 5.32 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
eb93136+dirty 416.18 ms 467.32 ms 51.14 ms
5257d80+dirty 423.37 ms 467.54 ms 44.17 ms
ca9d079+dirty 411.29 ms 455.12 ms 43.83 ms
7887847+dirty 416.61 ms 462.04 ms 45.43 ms
1122a96+dirty 422.22 ms 464.33 ms 42.10 ms
88735e9+dirty 429.04 ms 484.17 ms 55.13 ms
5fe1c6c+dirty 401.62 ms 445.28 ms 43.66 ms
4953e94+dirty 442.02 ms 456.52 ms 14.50 ms
ecf47a2+dirty 420.40 ms 458.02 ms 37.62 ms
5a010b7+dirty 425.62 ms 469.38 ms 43.76 ms

App size

Revision Plain With Sentry Diff
eb93136+dirty 48.30 MiB 53.58 MiB 5.28 MiB
5257d80+dirty 48.30 MiB 53.58 MiB 5.28 MiB
ca9d079+dirty 48.30 MiB 53.58 MiB 5.28 MiB
7887847+dirty 49.74 MiB 54.81 MiB 5.07 MiB
1122a96+dirty 48.30 MiB 53.54 MiB 5.24 MiB
88735e9+dirty 49.74 MiB 54.82 MiB 5.07 MiB
5fe1c6c+dirty 43.75 MiB 48.14 MiB 4.39 MiB
4953e94+dirty 43.75 MiB 48.08 MiB 4.33 MiB
ecf47a2+dirty 49.74 MiB 54.82 MiB 5.07 MiB
5a010b7+dirty 48.30 MiB 53.58 MiB 5.28 MiB

@sentry

sentry Bot commented Jun 22, 2026

Copy link
Copy Markdown

📲 Install Builds

Android

🔗 App Name App ID Version Configuration
Sentry RN io.sentry.reactnative.sample 8.15.1 (93) Release

⚙️ sentry-react-native Build Distribution Settings

Comment thread packages/core/android/CMakeLists.txt Outdated
@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 3837.96 ms 1214.96 ms -2623.00 ms
Size 4.98 MiB 6.51 MiB 1.53 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d038a14+dirty 3845.71 ms 1228.11 ms -2617.59 ms
41d6254+dirty 3845.71 ms 1224.51 ms -2621.20 ms
4966363+dirty 3854.04 ms 1231.55 ms -2622.50 ms
c004dae+dirty 3850.32 ms 1227.79 ms -2622.53 ms
eb93136+dirty 3843.09 ms 1220.11 ms -2622.98 ms
71abba0+dirty 3821.93 ms 1202.81 ms -2619.12 ms
ad66da3+dirty 3820.96 ms 1214.43 ms -2606.52 ms
ca9d079+dirty 3835.63 ms 1218.68 ms -2616.95 ms
df5d108+dirty 1225.90 ms 1220.14 ms -5.76 ms
4b87b12+dirty 1212.90 ms 1222.09 ms 9.19 ms

App size

Revision Plain With Sentry Diff
d038a14+dirty 5.15 MiB 6.67 MiB 1.51 MiB
41d6254+dirty 5.15 MiB 6.70 MiB 1.54 MiB
4966363+dirty 5.15 MiB 6.68 MiB 1.53 MiB
c004dae+dirty 5.15 MiB 6.67 MiB 1.51 MiB
eb93136+dirty 5.15 MiB 6.69 MiB 1.53 MiB
71abba0+dirty 5.15 MiB 6.67 MiB 1.52 MiB
ad66da3+dirty 5.15 MiB 6.67 MiB 1.51 MiB
ca9d079+dirty 5.15 MiB 6.69 MiB 1.53 MiB
df5d108+dirty 3.38 MiB 4.73 MiB 1.35 MiB
4b87b12+dirty 3.38 MiB 4.77 MiB 1.39 MiB

…debug info

Address Warden's medium-severity finding on #6307: passing
`-Wl,--strip-all` at CMake link time strips DWARF (and `.symtab`)
from `libsentry-tm-perf-logger.so` *before* AGP's `StripDebugSymbolsTask`
gets a chance to copy the unstripped artefact for symbolication
upload. Any crash inside the library in production would be
unsymbolicated even with the Sentry Gradle plugin installed.

Drop the manual link option entirely. AGP already strips the .so for
the packaged APK while preserving the unstripped copy under
`intermediates/merged_native_libs/.../obj`, which is the one Sentry
Gradle plugin uploads. Verified locally with `llvm-readelf -S` on the
release intermediate: `.debug_*` and `.symtab` sections are now
present.
Comment thread packages/core/ios/RNSentry.mm Outdated
alwx added 2 commits June 23, 2026 14:34
CI was failing 2 tests in `wrapTurboModule.test.ts` ("tracker push\nthrows" and "tracker pop throws"): the spy on `scope.setContext` was\nnever fired, so `pushTurboModuleCall` never threw, so the diagnostic\n`warn` call the tests asserted on never happened.\n\nRoot cause: commit `fix(turbomodule): Default TM tracker to isolation\nscope for native sync` (`8a16f7de`) switched `pushTurboModuleCall`'s\ndefault scope from `getCurrentScope()` to `getIsolationScope()` and\nupdated `turboModuleTracker.test.ts` to mock the new entry point, but\nmissed the sibling `wrapTurboModule.test.ts` which still mocked only\n`getCurrentScope`. The wrapper therefore wrote context/tags onto the\nreal isolation scope, the test's mock\u2019d `scope.setContext` never\nfired, and the assertions about the diagnostic warn went un-met.\n\nMock both `getIsolationScope` and `getCurrentScope` so the test\nremains deterministic regardless of which scope the tracker walks.\nAll 1618 JS tests pass after the change.
@alwx

alwx commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@antonis this one is ready to be reviewed again

Comment thread packages/core/cpp/SentryTurboModulePerfLogger.cpp
…td::terminate

Cursor HIGH on #6307: `SentryTurboModulePerfController::install` is\ndeclared `noexcept`, but it calls `std::make_unique<ForwardingLogger>()`\n(can throw `std::bad_alloc`) and forwards to\n`facebook::react::TurboModulePerfLogger::enableLogging` (no exception\nguarantees). If either throws \u2014 most plausibly under memory pressure\nwhen a user opts in to TurboModule tracking \u2014 the runtime calls\n`std::terminate` and the host app dies, instead of gracefully degrading\nto tracking-off.\n\nWrap the allocation+install in a `try { } catch (...) { }` and roll\nback the `installed_` latch on failure so a later `setEnabled(true)`\ncan retry once memory pressure clears. The SDK keeps running; the\nworst-case observable effect is that tracking remained off when the\nuser opted in. That is strictly better than terminating the process.\n\n`setEnabled` is unchanged \u2014 it only writes atomics and calls `install`,\nwhich is now truly `noexcept` after this fix.\n\nThe other comment in this review pass (Sentry bot MEDIUM about a\nconcurrent enable/disable race in `RNSentryTurboModulePerfTracker.setEnabled`)\nwas already addressed in commit 482ac45, which made `setEnabled`\n`synchronized` so the short-circuit and the lazy `System.loadLibrary`\nshare a single monitor.
Comment thread packages/core/RNSentryCocoaTester/RNSentryCocoaTester.xcodeproj/project.pbxproj Outdated
Comment thread packages/core/cpp/SentryTurboModulePerfLogger.cpp

@antonis antonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for iterating on this Alex 🙇 Left a few comments but overall looks good. I think the main remaining issue is figuring out the Android build failure on RN 0.71.9.

Two small follow-ups on #6307:

- Warden flagged the 23-character placeholder UUIDs I hand-rolled
  (`A1B2C3D4E5F600000000001` / `A1B2C3D4E5F600000000002`) in the
  RNSentryCocoaTester pbxproj when adding the new
  `RNSentryTurboModulePerfControllerTests.mm` source. Xcode expects
  exactly 24 uppercase hex characters; antonis confirmed. Replaced
  with real 24-character UUIDs (`2639D71D3BD04F17B0BAC987` /
  `E795057A6D534A80A9D06356`) across all four references
  (`PBXBuildFile`, `PBXFileReference`, `PBXSourcesBuildPhase`, and
  the group children).

- clang-format violation on the wrapped `enableLogging(...)` call in
  the noexcept-install fix \u2014 `yarn fix:clang` realigns it. CI lint
  job (run 28083145588) now matches local output.

Verified with `pod install` + `xcodebuild test` on the cocoa-tester
target: all 7 `RNSentryTurboModulePerfControllerTests` pass.
Comment thread packages/core/ios/RNSentry.mm Outdated
Comment thread packages/core/cpp/SentryTurboModulePerfLogger.cpp
Comment thread packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java Outdated
@alwx

alwx commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

it's getting huge and it's becoming hard to iterate on it — I will fix the remaining issues and blockers, and then we can move the remaining ones (if there are any) to a separate issue since this particular PR is kinda useless on its own anyway and is just among the first stages of Turbo Mobules integration project

cc @antonis

@antonis

antonis commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Sounds good @alwx 👍 It's getting hard to review too. Feel free to brake it down to separate PRs or chained PRs if that works better for you.

…ll race

Four fixes from PR #6307 review:

1. **antonis blocker** (RN 0.71.19 legacy Android matrix): the build
   failed with `CMake Error at CMakeLists.txt:21 (add_library): Target
   'sentry-tm-perf-logger' links to target 'ReactAndroid::reactnative'
   but the target was not found.` even though my gradle conditional
   only declared `externalNativeBuild` under NewArch. The cause is
   AGP/CMake auto-detection of `CMakeLists.txt` at the module root.
   Move the file to `src/main/jni/CMakeLists.txt` (next to OnLoad.cpp)
   so it lives outside any auto-detected path; gradle now references it
   explicitly via `cmake { path "src/main/jni/CMakeLists.txt" }` and
   the Old Arch build no longer touches it. Verified locally by running
   `./gradlew :sentry_react-native:assembleRelease` from
   `performance-tests/TestAppSentry/android` (newArchEnabled=false):
   no `configureCMake` task runs, no .so is produced, build succeeds.

2. **Warden** (`RNSentryModuleImpl.java:229`): if
   `RNSentryTurboModulePerfTracker.setEnabled` threw anything other
   than the already-caught `UnsatisfiedLinkError` (e.g.
   `SecurityException` from `System.loadLibrary`, any
   `RuntimeException` from the JNI symbol), execution skipped past
   `promise.resolve(true)` and the JS-side `initNativeSdk` promise
   hung forever. Wrap the `setEnabled` call in its own `try/catch`
   that logs and continues \u2014 the SDK has already started by this
   point, so a tracking-toggle failure is non-fatal to init.

3. **Cursor** (cross-platform parsing inconsistency): iOS accepted any
   `NSNumber` as the option value (so JS numeric `1` enabled
   tracking), Android required `ReadableType.Boolean` (so JS `1` was
   ignored). Tighten iOS to match: only honour an NSNumber whose
   `objCType` is `@encode(BOOL)`, which is what RN's bridge produces
   for a real JS boolean. JS numbers and strings now consistently fail
   the check on both platforms.

4. **Sentry bot** (race in `SentryTurboModulePerfController::install`):
   the previous fix rolled back `installed_` on `enableLogging`
   failure so a later caller could retry. That introduced a race: a
   concurrent thread observing the brief `installed_ == true` window
   would skip its own install attempt, then the originating thread's
   rollback would put us in a state where every caller thought someone
   else installed but nobody actually did. Switch to sticky
   "install attempted" semantics \u2014 the latch never rolls back. A
   failed install during the user opt-in path leaves tracking off for
   the rest of the process, which is strictly better than a silent
   half-installed state.

All builds + tests pass locally (`yarn lint`, samples on both arches,
`RNSentryAndroidTester`, `RNSentryCocoaTester/RNSentryTurboModulePerfControllerTests`).
@alwx

alwx commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@antonis ok, then please re-check it when you have time

Comment thread packages/core/ios/RNSentry.mm Outdated
… for option parsing

Warden caught a real bug in my previous `enableTurboModuleTracking`\nparsing fix: on 64-bit iOS `BOOL` is `typedef bool BOOL`, so\n`@encode(BOOL)` expands to `"B"` \u2014 but `[NSNumber numberWithBool:YES]`\n(including every JS boolean crossing the RN bridge) always reports\n`objCType == "c"` for historical compatibility. The\n`strcmp([(NSNumber *)value objCType], @encode(BOOL))` check therefore\nnever matched on any modern iOS device, and\n`enableTurboModuleTracking: true` was a silent no-op on iOS.\n\nReplace with the canonical, toll-free-bridged check\n`CFGetTypeID == CFBooleanGetTypeID()`. Verified with a small repro\nthat `@YES` returns `true` and `@1` returns `false` from\n`CFBooleanGetTypeID()`.\n\nAlso extract the parsing into a testable class method\n(`+ [RNSentry turboModuleTrackingEnabledFromOptions:]`) and add 7 unit\ntests in `RNSentryTurboModulePerfControllerTests.mm` covering:\n\n  - JS `true` \u2192 enabled (the bug)\n  - JS `false` \u2192 disabled\n  - JS `1` \u2192 disabled (cross-platform parity with Android's\n    `ReadableType.Boolean`)\n  - JS `0` \u2192 disabled\n  - String \u2192 disabled\n  - Missing key \u2192 disabled\n  - `NSNull` \u2192 disabled\n\nDictionary literals are hoisted into locals inside each test because\n`XCTAssertTrue`/`Fals`'s macro expansion uses `catch(T)` in ObjC++ and\nthe parser otherwise chokes on the comma inside `@{ k : v, }`.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 416006d. Configure here.

Comment thread packages/core/cpp/SentryTurboModulePerfLogger.cpp
Comment thread packages/core/android/build.gradle Outdated
…ode fallback error

Two follow-ups from latest review pass:

- **Cursor MEDIUM** (`SentryTurboModulePerfLogger.cpp`): the previous
  sticky-install fix accidentally let `isEnabled()` lie. The flow was
  `setEnabled(true)` \u2192 `enabled_ = true` \u2192 `install()` \u2192
  `enableLogging` throws \u2192 `installed_` stays sticky-true (no retry)
  \u2192 `isEnabled()` returns `true` even though RN never received the
  perf logger. Worse, a later `setEnabled(true)` would short-circuit
  on the sticky latch and the same lie persisted for the rest of the
  process.

  Split the latch into `installAttempted_` (sticky after the first
  try) and `installed_` (only `true` if `enableLogging` succeeded),
  and have `isEnabled()` AND user intent with actual install state.
  Now: a successful install \u2192 `isEnabled()` mirrors `enabled_`. A
  failed install \u2192 `isEnabled()` is permanently `false` for the
  process, even if the user keeps calling `setEnabled(true)`. This is
  strictly honest about what tracking can deliver and means tests and
  consumers stop seeing false-positive 'tracking is on' readings.

- **Sentry bot MEDIUM** (`build.gradle`): `resolveReactNativeDir()`
  calls `providers.exec("node", ...).get()` at gradle configure time,
  which throws hard if `node` is not on PATH. Wrap with a clearer
  `GradleException` pointing at the `REACT_NATIVE_NODE_MODULES_DIR`
  ext property the host project can use to skip the node lookup.

- **Warden** (RNSentry.mm BOOL parsing): the new comment was a snapshot
  of the bug that 416006d already fixed via `CFBooleanGetTypeID`.
  No code change needed beyond a reply pointing at that commit.
@alwx

alwx commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@antonis ok, that's ready now. I fixed a couple of Cursor and Sentry Warden comments but those are getting a bit ridiculous at this point (like no human would ever consider cases when node is not available in the RN app 😅)

@antonis

antonis commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@alwx Thanks for moving this work forward 🙏 Did another pass and I think the only blocking thing is the build failures

I will fix the remaining issues and blockers, and then we can move the remaining ones (if there are any) to a separate issue since this particular PR is kinda useless on its own anyway and is just among the first stages of Turbo Mobules integration project

Wdyt of using a feature branch and merge this PR on it? We can open separate issues for any pending items and move this forward. I usually don't like feature branches but since there is a lot of work on this project it might be beneficial.

…lability

Address the RN 0.71.19 legacy CI failure. Root cause:

  `dev-packages/e2e-tests/cli.mjs` has a long-standing bug: the check
  `if (env.RCT_NEW_ARCH_ENABLED)` is truthy for the string "0", so
  the legacy matrix entries (which set the env var to "0") actually
  flip `newArchEnabled=true` in the host app's `gradle.properties`.
  That has been silently masking new-arch coverage as legacy on every
  PR for ages; my CMake config is just the first thing that requires
  the `ReactAndroid::reactnative` prefab, which only exists in RN
  >= 0.75. So configure fails on RN 0.71.19 with:

      CMake Error at CMakeLists.txt:27 (add_library):
        Target "sentry-tm-perf-logger" links to target
        "ReactAndroid::reactnative" but the target was not found.

Rather than touch the CI script (out of this PR's scope), gate our
CMake config on the host's actual React Native version. The new
`isReactNativePrefabAvailable()` helper:

  1. requires `newArchEnabled=true` (same as before)
  2. resolves the host's `react-native/package.json` (via the existing\n     `resolveReactNativeDir()` helper, which honours\n     `REACT_NATIVE_NODE_MODULES_DIR`)
  3. returns true only when the major.minor is >= 0.75

Both `buildFeatures { prefab true }` and `externalNativeBuild { cmake\n... }` blocks are now keyed off this gate. On RN < 0.75 we skip the
native build entirely; `RNSentryTurboModulePerfTracker.setEnabled`
catches the missing `.so` via its existing `UnsatisfiedLinkError`
latch, exactly as it already does on Old Architecture.

Verified locally:

  - RN 0.86.0 + NewArch  \u2192 prefab-available = true,  4 .so files in AAR
  - RN 0.71.11           \u2192 prefab-available = false, no .so files
  - Android unit tests   \u2192 still pass
@alwx alwx requested a review from antonis June 24, 2026 21:19
// RN 0.75+ ships the `ReactAndroid::reactnative` prefab. Anything
// earlier is a legacy-only target as far as our native code is
// concerned.
return major > 0 || minor >= 75

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helpful GradleException silently swallowed, causing the native build to be invisibly skipped

When node is not on PATH and REACT_NATIVE_NODE_MODULES_DIR is not set, resolveReactNativeDir() throws a GradleException with a clear diagnostic message, but isReactNativePrefabAvailable() catches all Exceptions and returns false. On a new-arch RN 0.75+ project where Node isn't in Gradle's PATH, the native library silently won't be built and the TurboModule perf logger will be a silent no-op with no indication of why.

Evidence
  • resolveReactNativeDir() (line ~62–73) throws GradleException with a detailed message when node is unavailable and no override is set.
  • isReactNativePrefabAvailable() wraps its entire body in catch (Exception ignored) { return false } (line 37), which catches GradleException (a subclass of RuntimeExceptionException).
  • If isReactNativePrefabAvailable() returns false, the externalNativeBuild and cmake blocks in defaultConfig are never added, so libsentry-tm-perf-logger.so is never compiled.
  • The user sees no error or warning; the feature is silently absent.

Identified by Warden code-review · WX6-4DK

* start/end/fail, execution start/end/fail) to the higher-level Sentry
* instrumentation (crash attribution, per-module spans, aggregated stats).
*
* Only takes effect on React Native New Architecture. On Old Architecture

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: It is also 0.75+ right?

Suggested change
* Only takes effect on React Native New Architecture. On Old Architecture
* Only takes effect on React Native 0.75+ New Architecture. On Old Architecture

@antonis antonis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM after resolving the conflicts. I think the failed checks are just flakes now (rerun 🤞)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Triggers the full CI test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire TurboModulePerfLogger on iOS and Android

3 participants